Skip to content

[Testing] Restructure/rewrite testing docs #14731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 17, 2021
Merged

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Dec 22, 2020

These are my first ideas for a better testing documentation. One of the major results of the Testing survey (and Testing initiative of @Nyholm, @weaverryan and @kbond) was a lack of good documentation beyond the basic stuff.

This is a very very rough sketch of my ideas. I've mostly moved sections around and added some titles. So the actual content probably no longer reads as one article, which is going to be fixed before marking this PR as finished.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work. Well done.

I had a few comments. You probably already know but I just wanted to share.

testing.rst Outdated
``PostController`` class, start by creating a new ``PostControllerTest.php``
file that extends a special ``WebTestCase`` class.

As an example, a test could look like this::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about referencing make:functional-test and make:unit-test from the https://github.com/symfony/maker-bundle?

can be a good bootstraping code, like for fixtures above :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, these commands are now deprecated and a new make:test command has been introduced: https://github.com/symfony/maker-bundle/blob/main/src/Maker/MakeTest.php#L38-L42

IMHO the docs should be consistent with this command, and explain all TestCase types that are available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. For now, I've updated the "Application Tests" section to use the new make:test command. Let's rework this thing in a follow-up PR.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Here are some minor comments.


.. code-block:: terminal

$ composer require --dev symfony/css-selector
$ composer require --dev dama/doctrine-test-bundle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add this package to test-pack then?

Copy link
Member

@kbond kbond Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to recommend dama/doctrine-test-bundle as a performance tool rather than a reset tool. Transactions can be daunting to newcomers and can make debugging a failing test difficult (the data isn't in your test database after a test fails).

I think the plan is to add zenstruck/foundry to the test-pack and recommend using it's ResetDatabase trait (which doesn't, by default, rely on transactions to reset).

(this comment is a response to #14731 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. However, as we know, there is no official reset tool for Doctrine, nor is there a library that only does the reset logic. So we'll have to wait for Foundry to be documented in this article before we can recommend the ResetDatabase trait for resetting and only use the dama bundle for performance.

Also, this PR targets 4.4 (to avoid merge conflicts for the next years). We'll probably only want to talk about Foundry in the 5.3+ version of the docs.


.. versionadded:: 4.3
That's it! This bundle uses a clever trick: it begins a database
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will probably break tests using Panther. Can this be disabled on a per-test basis?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be disabled on a per-test basis?

Theoretically its possible. But this would defeat the purpose a bit since then some tests will actually persist changes in the DB and potentially impact other tests.

In this case I think one should split the tests into different suites and only enable dama/doctrine-test-bundle for non-Panther tests maybe.

testing.rst Outdated
``PostController`` class, start by creating a new ``PostControllerTest.php``
file that extends a special ``WebTestCase`` class.

As an example, a test could look like this::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, these commands are now deprecated and a new make:test command has been introduced: https://github.com/symfony/maker-bundle/blob/main/src/Maker/MakeTest.php#L38-L42

IMHO the docs should be consistent with this command, and explain all TestCase types that are available.

testing.rst Outdated

.. index::
pair: PHPUnit; Configuration
TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add a section about ApiTestCase (or a link to this page)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's do that in a follow-up PR :)

@Nyholm
Copy link
Member

Nyholm commented Mar 9, 2021

Awesome. Thank you Kevin!
I agree with all your comments. @wouterj I'll let you decide what actions to take on Kevin's review.

My suggestion is that we should try to wrap up this PR to get it merged (hopefully this week) and I'll be happy to take actions on the comments in a separate PR.

fabpot added a commit to symfony/symfony that referenced this pull request Mar 12, 2021
…Nyholm)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[FrameworkBundle] Add KernelTestCase::getContainer()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       |
| License       | MIT
| Doc PR        | symfony/symfony-docs#14731

There are at least 3 ways to get the container in a test class:

```php
class FooTest extends WebTestCase
{
    public function testGetContainerA()
    {
        $kernel = self::bootKernel();
        $container = $kernel->getContainer();
    }

    public function testGetContainerB()
    {
        self::bootKernel();
        $container = self::$container;
    }

    public function testGetContainerC()
    {
        $client = self::createClient();
        $container = $client->getContainer();
    }
}
```

I suggest to add a fourth =)

Basically, in tests you should always use the `test.service_container`. It is hard to remove A and C, but I can deprecate C and add a helper function.

```php
class FooTest extends WebTestCase
{
    public function testGetContainerTheOnlyWayYouShouldUse()
    {
        $container = $this->getContainer();
    }
}
```

This new way will also boot your kernel if it is not already booted.

Commits
-------

f4c9724 [FrameworkBundle] Add KernelTestCase::getContainer()
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Mar 12, 2021
…Nyholm)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[FrameworkBundle] Add KernelTestCase::getContainer()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       |
| License       | MIT
| Doc PR        | symfony/symfony-docs#14731

There are at least 3 ways to get the container in a test class:

```php
class FooTest extends WebTestCase
{
    public function testGetContainerA()
    {
        $kernel = self::bootKernel();
        $container = $kernel->getContainer();
    }

    public function testGetContainerB()
    {
        self::bootKernel();
        $container = self::$container;
    }

    public function testGetContainerC()
    {
        $client = self::createClient();
        $container = $client->getContainer();
    }
}
```

I suggest to add a fourth =)

Basically, in tests you should always use the `test.service_container`. It is hard to remove A and C, but I can deprecate C and add a helper function.

```php
class FooTest extends WebTestCase
{
    public function testGetContainerTheOnlyWayYouShouldUse()
    {
        $container = $this->getContainer();
    }
}
```

This new way will also boot your kernel if it is not already booted.

Commits
-------

f4c97240ff [FrameworkBundle] Add KernelTestCase::getContainer()
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Mar 12, 2021
…Nyholm)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[FrameworkBundle] Add KernelTestCase::getContainer()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       |
| License       | MIT
| Doc PR        | symfony/symfony-docs#14731

There are at least 3 ways to get the container in a test class:

```php
class FooTest extends WebTestCase
{
    public function testGetContainerA()
    {
        $kernel = self::bootKernel();
        $container = $kernel->getContainer();
    }

    public function testGetContainerB()
    {
        self::bootKernel();
        $container = self::$container;
    }

    public function testGetContainerC()
    {
        $client = self::createClient();
        $container = $client->getContainer();
    }
}
```

I suggest to add a fourth =)

Basically, in tests you should always use the `test.service_container`. It is hard to remove A and C, but I can deprecate C and add a helper function.

```php
class FooTest extends WebTestCase
{
    public function testGetContainerTheOnlyWayYouShouldUse()
    {
        $container = $this->getContainer();
    }
}
```

This new way will also boot your kernel if it is not already booted.

Commits
-------

f4c97240ff [FrameworkBundle] Add KernelTestCase::getContainer()
@OskarStark
Copy link
Contributor

@dmaicher maybe you want to have look at his PR 👀

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few minor comments.


.. code-block:: terminal

$ composer require --dev symfony/css-selector
$ composer require --dev dama/doctrine-test-bundle
Copy link
Member

@kbond kbond Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to recommend dama/doctrine-test-bundle as a performance tool rather than a reset tool. Transactions can be daunting to newcomers and can make debugging a failing test difficult (the data isn't in your test database after a test fails).

I think the plan is to add zenstruck/foundry to the test-pack and recommend using it's ResetDatabase trait (which doesn't, by default, rely on transactions to reset).

(this comment is a response to #14731 (comment))

namespace App\Tests\Controller;
If you need to customize some environment variables for your tests (e.g. the
``DATABASE_URL`` used by Doctrine), you can do that by overriding anything you
need in your ``.env.test`` file:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment variables can also be added to your phpunit.xml. When would you use phpunit.xml over .env.test? Is there a reason these two env variables can't be added to .env.test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I think they can be part of .env.test. @nicolas-grekas do you maybe know why they aren't?

wouterj and others added 2 commits April 17, 2021 13:10
* Moved around things
* Don't mention PHPUnitBridge
* Add info about integration tests
@wouterj wouterj force-pushed the testing branch 3 times, most recently from 78d3e42 to e114598 Compare April 17, 2021 13:04
@wouterj wouterj marked this pull request as ready for review April 17, 2021 13:04
@carsonbot carsonbot added this to the 4.4 milestone Apr 17, 2021
@carsonbot carsonbot changed the title [WIP][Testing] Restructure/rewrite testing docs [Testing] [WIP] Restructure/rewrite testing docs Apr 17, 2021
@wouterj wouterj changed the title [Testing] [WIP] Restructure/rewrite testing docs [Testing] Restructure/rewrite testing docs Apr 17, 2021
@wouterj
Copy link
Member Author

wouterj commented Apr 17, 2021

I think this PR is now ready to be merged. The rewrite is not fully finished, but finished enough to be merged imho. Merging this now allows Tobias and others to also work on the document.

Among some other things, my major open points with this article are: move most of the dom crawler section to a subarticle and favor the Symfony assertion traits; use make:test in all sections; document the other options of make:test.

@javiereguiluz or @weaverryan do you mind having a look at this PR (and maybe merge it)?

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a fantastic work! Thanks a lot for the effort put in this task.

@wouterj wouterj merged commit 6d99029 into symfony:4.4 Apr 17, 2021
@wouterj
Copy link
Member Author

wouterj commented Apr 17, 2021

Aaand it's merged. Thanks for your blazing fast response on this Saturday afternoon @javiereguiluz!

And a special big thanks to @Nyholm for doing the bulk of the work (including pursuing me to finally finish this PR).

@wouterj wouterj deleted the testing branch April 17, 2021 15:48
@Nyholm
Copy link
Member

Nyholm commented Apr 17, 2021

What?! This is awesome. Thank you all for the help with this <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants